-
Notifications
You must be signed in to change notification settings - Fork 59
feat: CLI prove use bin name for output path #1675
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: CLI prove use bin name for output path #1675
Conversation
Options `--manifest-path`, `--target-dir` are also available to `verify`. If you omit `--app_vk` and/or `--proof`, the command will search for those files at `${target_dir}/openvm/app.vk` and `./app.proof` respectively. | ||
Options `--manifest-path`, `--target-dir` are also available to `verify`. If you omit `--app_vk` the command will search for the verifying key at `${target_dir}/openvm/app.vk`. | ||
|
||
If you omit `--proof`, the command will search the working directory for files with the `.app.proof` extension. Note that for this default case a single proof is expected to be found, and `verify` will fail otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think verify
should always pass in --proof
to be explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily opposed to this, but our CLI toolchain generally has the semantic where we allow file inputs to be omitted if there's exactly one option. Given that we error out if there isn't exactly one option, I think it might be good to maintain this behavior in verify
. Lmk what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when you do prove
, because the input (exe) is a generated, and is never really being "used" directly outside of the openvm context, we can use some reasonable default.
But I think the "proof" is a bit different in that it's the final object that when will used elsewhere (onchain?) so would make sense to be explicit. Want to avoid the case that user think a proof is verified, but actually they verify something else (on the default path)
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From earlier discussions with Jonathan, I think we should assume that all of the generated artifacts (i.e. .pk
, .vk
, .vmexe
, etc.) can and likely will be used somewhere. Default naming is done using the project's structure, which the user should in theory know about and set.
I don't think it's a stretch to assume that users will be able to deduce that a verify
call with no --proof
specified will look in the working directory, especially since that's the output behavior of prove
? In general I feel like if a user does prove --proof file.proof
they should know to do verify --proof file.proof
.
That being said, regardless of what we choose we should probably output a success message with the proof path. As of now there's no messaging, which does indeed make it difficult to confirm what's going on 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i think printing the proof path is a good idea. ok i think this is probably fine
Commit: f6a2562 |
Resolves INT-4074. Primarily accomplishes the following:
cargo openvm prove
outputs proofs to${bin_name}.app.proof
instead ofapp.proof
, wherebin_name
is the file stem of the executable (same forstark
andevm
)cargo openvm verify
by default searches the working directory for files with extension.app.proof
Additionally, the following are also in this PR:
cargo openvm init ${dir}
creates directorydir
if it doesn't already existThe following were merged from #1689:
cargo openvm verify stark
subcommand